-
-
Notifications
You must be signed in to change notification settings - Fork 375
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feature: Prerender using webpack #71
feature: Prerender using webpack #71
Conversation
A thought I had: could we produce a UMD bundle that would be usable in both the client and the server? Definitely interested in any way to avoid the overhead of 2 concurrent builds. I think the closest prior art I know of here would be static-site-generator-webpack-plugin - maybe their technique would work. Or maybe a webpack bundle that can detect if its running under node/cjs and do a |
src/lib/run-webpack.js
Outdated
newConfig.output.libraryTarget = 'commonjs2'; | ||
|
||
let asyncLoaderIndex = newConfig.module.loaders | ||
.findIndex(l => l.loader === path.resolve(__dirname, 'async-component-loader')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might be able to just alias this to a dummy / passthrough loader
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it would be better + whenever someone uses import Component from 'async!./components/...'
currently it won't work
If you don't mind I might also take a stab at this with the UMD approach. I'm curious, but it might not work at all so I don't want to waste your time 😇 |
@developit go ahead! 😄 Also, I'm thinking about using some in memory file system (memfs for example) for webpack builds so that there won't be some magic folder called in npm module (+using normal fs breaks concurrency) 😄 |
@developit I didn't see your first comment.
Technically possible. Passing two configs to webpack with two entries (sth like |
I've been able to pinpoint location of the erroneous plugin 😄, this: goldhand/sw-precache-webpack-plugin#78 will enable us to use single webpack compiler pass & reduce overhead. 🎉 |
…lient & server separate configs
All wrapped up & tested. @developit if you haven't started yet & have some ideas for further work on this, feel free to work on this branch. 😄 I'm thinking about writing some tests that would be able to verify the CLI works correctly (not only running builds, but running built bundles & watch mode). |
@rkostrzewski I think we should bump the sw-precache dependency now that your PR there has been merged. |
I haven't tried the code yet, but is polyfilling |
# Conflicts: # src/lib/run-webpack.js
@kurtextrem During SSR, only @rkostrzewski I didn't get a chance to test out yet, so I'll just check this branch out and try some things. |
|
||
customConfig({ | ||
resolve: { | ||
modules: [ | ||
'node_modules', | ||
resolve(__dirname, '../../node_modules') | ||
resolve(__dirname, '../../../node_modules') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should just be using NODE_PATH for this wherever possible. Actually, anywhere we have node_modules references for resolving, we should add NODE_PATH. That'll make us future proof.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a variable inside the file const NODE_PATH = resolve(__dirname, '../../node_modules')
or via CLI?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought NODE_PATH
pointed to the global node_modules
directory?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@developit Correct, which is what you want. It's basically used when people decide not to put node_modules inside their root dir but rather somewhere else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right but that assumes global - preact-cli installs itself as a local dep and this is looking there. I had wondered if there was a way to get to that path... maybe something like:
path.join(require.resolve('preact-cli'), 'node_modules')
Refactor, testing build size
Headless chrome, all commands tested
# Conflicts: # src/commands/build.js # src/commands/watch.js
CR changes
just an fyi, i just debugged |
# Conflicts: # .gitignore
# Conflicts: # .travis.yml
I just tried this branch on my site and I got the following error:
|
# Conflicts: # src/commands/watch.js # src/lib/prerender.js # src/lib/webpack/webpack-base-config.js
LGTM! I just went through and tested this super thoroughly. Also tried using the few config options differently for templates and such. :) |
# Conflicts: # package.json # tests/build.snapshot.js # tests/build.test.js # tests/lib/chrome.js # tests/lib/cli.js # tests/lib/output.js # tests/serve.test.js
# Conflicts: # src/lib/webpack/webpack-base-config.js
# Conflicts: # src/lib/webpack/webpack-base-config.js
# Conflicts: # src/lib/webpack/webpack-base-config.js
teaaaam let's put this in master yo |
@hassanbazzi +1 but looks like tests are failing? |
Seems like snapshots have different sizes on CI. |
# Conflicts: # package.json # tests/build.snapshot.js # tests/build.test.js
# Conflicts: # package.json # src/commands/build.js # src/commands/watch.js # src/lib/webpack/webpack-base-config.js # tests/build.snapshot.js # tests/build.test.js
test(`preact build - should use custom .babelrc.`, options, async t => { | ||
// app with custom .babelrc enabling async functions | ||
let app = await fromSubject('custom-babelrc'); | ||
|
||
// UglifyJS throws error when generator is encountered | ||
// TODO: Remove '--no-prederender' once #71 is merged - babel-register problem | ||
await build(app, ['--no-prerender']); | ||
await build(app); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Got it to work with uglify?
LGTM :) |
Prerender fails when webpack specific stuff is in js code (described here #43, here #39, here #62 and possibly other places).
This fixes that by creating separate webpack bundle just for prerender with commonjs2 target.
This adds some overhead when building, but I couldn't use webpack's multiple compiler with two configs because some plugins failed. 😢
WIP because I must fully test this & wanted to hear early feedback.